Skip to content

plugins: fix compilation if FLB_SQLDB (sqlite3) is disabled #10239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

plugins: fix compilation if FLB_SQLDB (sqlite3) is disabled #10239

wants to merge 7 commits into from
+116 −13

Conversation

@ThomasDevoogdt
Copy link
Contributor

@ThomasDevoogdt ThomasDevoogdt commented Apr 22, 2025 •
edited by coderabbitai bot
Loading

Fixes:

Summary by CodeRabbit

  • New Features

    • SQL database support made optional across plugins; when available, partial-match checklist and enhanced blob upload tracking are enabled.
    • Added a new post-upload action: "Add Suffix".

  • Bug Fixes

    • Non-database builds now cleanly skip DB-dependent modes/events and return explicit errors for unsupported operations while preserving exact-match behavior.
    • Improved initialization/cleanup on failure to avoid leftover state.

  • Tests

    • CI and test suites extended to run both DB-enabled and DB-disabled variants.

@ThomasDevoogdt ThomasDevoogdt mentioned this pull request Apr 22, 2025
Open
@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/sqlite3-disabled branch from 4b2f2ad to a767a54 Compare April 23, 2025 09:43
@patrick-stephens patrick-stephens added ok-package-test Run PR packaging tests and removed docs-required labels Apr 24, 2025
@patrick-stephens
Copy link
Collaborator

patrick-stephens commented Apr 24, 2025

Let's make sure this compiles for all existing targets as well.

Would it be better to disable the features within the plugins that need the DB rather than the whole plugin @leonardo-albertovich ? It feels a bit like a large hammer, e.g. no tail input even if you don't use db. We would have to make the config options and any other usage conditional though so it may be worse.

@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented Apr 24, 2025

Let's make sure this compiles for all existing targets as well.

How can it not? Currently, FLB_SQLDB is enabled by default, and will stay like that. The problem is the other way around, if not enabled, then compilation breaks.

Would it be better to disable the features within the plugins that need the DB rather than the whole plugin @leonardo-albertovich ? It feels a bit like a large hammer, e.g. no tail input even if you don't use db. We would have to make the config options and any other usage conditional though so it may be worse.

For me fine, but I would do that on a per feature basis. Perhaps just continue with this PR, and then fine-tune some features that might compile with some small fixups.

In general, it would be much better if all options are toggled in the automated tests, on one reference compilation system. Perhaps even by incremental builds. But either way, I just want to fix compilation now.

@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/sqlite3-disabled branch from a767a54 to 8cf7987 Compare May 2, 2025 11:53
@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/sqlite3-disabled branch from 8cf7987 to 7092bc1 Compare May 2, 2025 11:54
@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented May 2, 2025 •
edited
Loading

@patrick-stephens @edsiper I changed this PR so that plugins are still compiled, but without database support. I hope this answers #10239 (comment).

Tested by doing this:

cd build/
cmake -GNinja -DFLB_SQLDB=OFF ../
ninja

and

cd build/
cmake -GNinja -DFLB_PREFER_SYSTEM_LIBS=ON -DFLB_SQLDB=OFF ../
ninja

@patrick-stephens
Copy link
Collaborator

patrick-stephens commented May 2, 2025

Not sure if the CI failure is relevant or something else

@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented May 2, 2025

Not sure if the CI failure is relevant or something else

I just added #ifdef FLB_HAVE_SQLDB, which is true by default, so I don't think it's relevant.

This was referenced May 5, 2025
Merged
Open
@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/sqlite3-disabled branch from 7092bc1 to 24532cd Compare May 7, 2025 07:28
@ThomasDevoogdt ThomasDevoogdt force-pushed the bugfix/sqlite3-disabled branch from a574d8a to 80cd590 Compare November 3, 2025 09:07
@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented Nov 3, 2025

This PR seems good but we're not able to add 👍 because of failing unit testing.

Branch is rebased, remaining coderabbitai remarks are fixed. Can you merge it now?
+cc @edsiper

@cosmo0920 cosmo0920 added this to the Fluent Bit v4.2 milestone Nov 4, 2025
@edsiper
Copy link
Member

edsiper commented Nov 4, 2025

I think in_blob, flb_blob_db must be disabled if sqlite is not available, since they must have the state.

Out S3 and out_azure will need to be aware of the lack of flb_blob_db so they can disable blob handling support

@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented Nov 4, 2025

@edsiper

I think in_blob, flb_blob_db must be disabled if sqlite is not available, since they must have the state.

But why do I then see a bunch of other FLB_HAVE_SQLDB declaratives in flb_blob_db?

e.g.

fluent-bit/plugins/in_blob/blob.c

Line 641 in ed06c31

#ifdef FLB_HAVE_SQLDB

If it has to be disabled, do you expect me to drop all existing FLB_HAVE_SQLDB declaratives? Because I somehow expect that the original code should just compile, but was somehow broken over time. What I try to fix now.

@leonardo-albertovich
Copy link
Contributor

leonardo-albertovich commented Nov 4, 2025

in_blob doesn't need to be disabled because it can operate without a database
flb_blob_db does't need to be excluded because when FLB_HAVE_SQLDB isn't detected it impleemnts the exact same functions except they all return FLB_BLOB_DB_ERROR_NO_BACKEND_AVAILABLE as the error code
out_azure_blob doesn't need to be disabled because when FLB_HAVE_SQLDB isn't detected it just doesn't process blobs
out_s3 doesn't need to be disabled because when FLB_HAVE_SQLDB isn't detected it just doesn't process blobs

That's of course as far as I know and as a side note, the test case issue is unrelated to this feature because it's flb-rt-core_chunk_trace.

@ThomasDevoogdt
Copy link
Contributor Author

ThomasDevoogdt commented Nov 4, 2025

in_blob doesn't need to be disabled because it can operate without a database flb_blob_db does't need to be excluded because when FLB_HAVE_SQLDB isn't detected it impleemnts the exact same functions except they all return FLB_BLOB_DB_ERROR_NO_BACKEND_AVAILABLE as the error code out_azure_blob doesn't need to be disabled because when FLB_HAVE_SQLDB isn't detected it just doesn't process blobs out_s3 doesn't need to be disabled because when FLB_HAVE_SQLDB isn't detected it just doesn't process blobs

That's of course as far as I know and as a side note, the test case issue is unrelated to this feature because it's flb-rt-core_chunk_trace.

Then I don't touch anything. I would appreciate that this can just be merged then. Thanks for the review!

@leonardo-albertovich
Copy link
Contributor

leonardo-albertovich commented Nov 4, 2025

Oh no, I didn't review the PR, I just clarified how things worked originally and what the CI issue was.

leonardo-albertovich
leonardo-albertovich suggested changes Nov 4, 2025
View reviewed changes
Copy link
Contributor

@leonardo-albertovich leonardo-albertovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a minor nitpick but overall it's good to go.

I think replicating the flb_blob_db mechanism where the functions are defined but all of them return a specific error in azure_blob_db.c would make more sense than adding most of the pre-processor conditionals in the callers but TBH that component as a whole should be replaced by flb_blob_db which is the reason why the functionality was extracted instead of implemented in out_s3 originally so I don't think it'd be reasonable to get too pedantic here.

plugins/out_azure_blob/azure_blob.c
return FLB_OK;
}

#ifdef FLB_HAVE_SQLDB
Copy link
Contributor

@leonardo-albertovich leonardo-albertovich Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this function wrapped by this conditional when it only executes an HTTP call?
Also, this function wouldn't be called because cb_azb_blob_file_upload would exit prematurely due to ctx->db being set to NULL

plugins/out_azure_blob/azure_blob.c
flb_plg_error(ctx->ins, "could not generate block id");
flb_free(generated_random_string);
cfl_sds_destroy(ref_name);
flb_sds_destroy(ref_name);
Copy link
Contributor

@leonardo-albertovich leonardo-albertovich Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct.
The only thing is bundling this fix with the FLB_SQLDB patch means it could slip through the cracks and not make it to the other branch that should receive the same patch.
I'm not saying you have to create a new PR, it's more of an internal note.

plugins/out_azure_blob/azure_blob_conf.c
}

pthread_mutex_init(&ctx->file_upload_commit_file_parts, NULL);
#endif
Copy link
Contributor

@leonardo-albertovich leonardo-albertovich Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional should be one line before, even though file_upload_commit_file_parts is used by a function that will exit prematurely there's no reason not to initialize it.

Copy link
Contributor Author

@ThomasDevoogdt ThomasDevoogdt Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 6df3149, file_upload_commit_file_parts is excluded by FLB_HAVE_SQLDB. So it doesn't make sense to suddenly move it just for the purpose of initializing it.

@edsiper
Copy link
Member

edsiper commented Nov 9, 2025

moving this until the next milestone, some parts are still moving

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@cosmo0920 cosmo0920 cosmo0920 approved these changes

@edsiper edsiper Awaiting requested review from edsiper edsiper is a code owner

@fujimotos fujimotos Awaiting requested review from fujimotos

@koleini koleini Awaiting requested review from koleini

@niedbalski niedbalski Awaiting requested review from niedbalski niedbalski is a code owner

@patrick-stephens patrick-stephens Awaiting requested review from patrick-stephens patrick-stephens is a code owner

@celalettin1286 celalettin1286 Awaiting requested review from celalettin1286 celalettin1286 is a code owner

@leonardo-albertovich leonardo-albertovich Awaiting requested review from leonardo-albertovich

Assignees

No one assigned

Labels

ok-package-test Run PR packaging tests

Projects

None yet

Milestone

Fluent Bit Next

Development

Successfully merging this pull request may close these issues.

5 participants

@ThomasDevoogdt @patrick-stephens @edsiper @leonardo-albertovich @cosmo0920